-
Notifications
You must be signed in to change notification settings - Fork 7
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add popup with unprocessed messages #603
Conversation
kongzii
commented
Dec 13, 2024
WalkthroughThe pull request introduces modifications to three files: Changes
Possibly related PRs
Suggested reviewers
Tip CodeRabbit's docstrings feature is now available as part of our Early Access Program! Simply use the command Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
transactions = blockchain_transaction_fetcher().fetch_unseen_transactions( | ||
nft_agent.wallet_address | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I click 2x on the button, does it trigger fetch_unseen_transactions
2x?
spice
is caching results, but still I would suggest placing the transactions
inside a st.session_state
, updating that through st.fragment
and just reading from that inside the popover. Wdyt?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ow, understood - so button simply toggles the disabled
property and shows stuff, but doesn't trigger any fetching logic. Fine.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (1)
prediction_market_agent/db/blockchain_transaction_fetcher.py (1)
Line range hint
44-70
: Improve performance by avoiding row-wise iteration over the DataFrameUsing
df.iter_rows(named=True)
for converting DataFrame rows toBlockchainMessage
objects can be inefficient for large datasets. Polars supports vectorized operations, which are more performant.Consider using
df.apply
or vectorized methods to convert the DataFrame to a list ofBlockchainMessage
objects without explicit iteration:return [ - self.blockchain_message_from_dune_df_row(consumer_address, x) - for x in df.iter_rows(named=True) + self.blockchain_message_from_dune_df_row(consumer_address, row) + for row in df.to_dicts() ]This approach leverages Polars' efficient data handling capabilities.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
prediction_market_agent/agents/microchain_agent/nft_treasury_game/app_nft_treasury_game.py
(4 hunks)prediction_market_agent/db/blockchain_transaction_fetcher.py
(4 hunks)
🔇 Additional comments (1)
prediction_market_agent/agents/microchain_agent/nft_treasury_game/app_nft_treasury_game.py (1)
205-207
: Optimize transaction fetching to avoid redundant API calls
Repeatedly calling fetch_unseen_transactions
inside the popover may lead to unnecessary API requests if the popover is opened multiple times. This can affect performance and user experience.
Consider storing the transactions
in st.session_state
or updating them through a st.fragment
, then reading from that inside the popover. This approach reduces redundant API calls and improves efficiency.
def blockchain_message_from_dune_df_row( | ||
self, consumer_address: ChecksumAddress, x: dict[str, Any] | ||
) -> BlockchainMessage: | ||
return BlockchainMessage( | ||
consumer_address=consumer_address, | ||
transaction_hash=x["hash"], | ||
value_wei=str(x["value"]), | ||
block=str(x["block_number"]), | ||
sender_address=x["from"], | ||
data_field=self.unzip_message_else_do_nothing(x["data"]), | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Handle potential KeyError
exceptions when accessing dictionary keys
In blockchain_message_from_dune_df_row
, accessing dictionary keys directly (e.g., x["hash"]
) without verifying their existence can lead to KeyError
exceptions if any keys are missing from the x
dictionary. This could cause the application to crash unexpectedly when processing incomplete data from external sources.
Consider modifying the method to handle missing keys gracefully:
def blockchain_message_from_dune_df_row(
self, consumer_address: ChecksumAddress, x: dict[str, Any]
) -> BlockchainMessage:
return BlockchainMessage(
consumer_address=consumer_address,
- transaction_hash=x["hash"],
- value_wei=str(x["value"]),
- block=str(x["block_number"]),
- sender_address=x["from"],
- data_field=self.unzip_message_else_do_nothing(x["data"]),
+ transaction_hash=x.get("hash", ""),
+ value_wei=str(x.get("value", 0)),
+ block=str(x.get("block_number", 0)),
+ sender_address=x.get("from", ""),
+ data_field=self.unzip_message_else_do_nothing(x.get("data", "")),
)
This ensures that missing keys default to safe values, preventing KeyError
exceptions.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
def blockchain_message_from_dune_df_row( | |
self, consumer_address: ChecksumAddress, x: dict[str, Any] | |
) -> BlockchainMessage: | |
return BlockchainMessage( | |
consumer_address=consumer_address, | |
transaction_hash=x["hash"], | |
value_wei=str(x["value"]), | |
block=str(x["block_number"]), | |
sender_address=x["from"], | |
data_field=self.unzip_message_else_do_nothing(x["data"]), | |
) | |
def blockchain_message_from_dune_df_row( | |
self, consumer_address: ChecksumAddress, x: dict[str, Any] | |
) -> BlockchainMessage: | |
return BlockchainMessage( | |
consumer_address=consumer_address, | |
transaction_hash=x.get("hash", ""), | |
value_wei=str(x.get("value", 0)), | |
block=str(x.get("block_number", 0)), | |
sender_address=x.get("from", ""), | |
data_field=self.unzip_message_else_do_nothing(x.get("data", "")), | |
) |
transactions = self.fetch_unseen_transactions(consumer_address=consumer_address) | ||
return len(transactions) | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Avoid fetching all transactions just to get the count
Calling fetch_unseen_transactions
to get the count of unprocessed transactions fetches all transaction data, which is inefficient and could impact performance.
Refactor fetch_count_unprocessed_transactions
to fetch only the count from the database:
def fetch_count_unprocessed_transactions(
self, consumer_address: ChecksumAddress
) -> int:
- transactions = self.fetch_unseen_transactions(consumer_address=consumer_address)
- return len(transactions)
+ keys = APIKeys()
+ latest_blockchain_message = (
+ self.blockchain_table_handler.fetch_latest_blockchain_message(
+ consumer_address
+ )
+ )
+ min_block_number = (
+ 0 if not latest_blockchain_message else latest_blockchain_message.block
+ )
+ query = f'SELECT COUNT(*) AS count FROM gnosis.transactions WHERE "to" = {Web3.to_checksum_address(consumer_address)} AND block_number >= {min_block_number} AND value >= {xdai_to_wei(MicrochainAgentKeys().RECEIVER_MINIMUM_AMOUNT)}'
+ df = spice.query(query, api_key=keys.dune_api_key.get_secret_value())
+ existing_hashes = self.blockchain_table_handler.fetch_all_transaction_hashes(
+ consumer_address=consumer_address
+ )
+ total_count = df.select("count")[0, 0]
+ unseen_count = total_count - len(existing_hashes)
+ return max(unseen_count, 0)
This change reduces unnecessary data retrieval and improves performance.
Committable suggestion skipped: line range outside the PR's diff.
transactions = self.fetch_unseen_transactions(consumer_address=consumer_address) | ||
if not transactions: | ||
return None | ||
|
||
# We only want the oldest non-processed message. | ||
oldest_non_processed_message = df.row(0, named=True) | ||
blockchain_message = BlockchainMessage( | ||
consumer_address=consumer_address, | ||
transaction_hash=oldest_non_processed_message["hash"], | ||
value_wei=str(oldest_non_processed_message["value"]), | ||
block=str(oldest_non_processed_message["block_number"]), | ||
sender_address=oldest_non_processed_message["from"], | ||
data_field=self.unzip_message_else_do_nothing( | ||
oldest_non_processed_message["data"] | ||
), | ||
) | ||
blockchain_message = transactions[0] | ||
|
||
# Store here to avoid having to refresh after session was closed. | ||
item = blockchain_message.model_copy(deep=True) | ||
# mark unseen transaction as processed in DB | ||
self.blockchain_table_handler.save_multiple([blockchain_message]) | ||
return item | ||
return blockchain_message |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Optimize fetching of a single unprocessed transaction
Fetching all unseen transactions and selecting the first one is inefficient, especially when only one transaction is needed.
Modify the method to fetch only the oldest unseen transaction:
def fetch_one_unprocessed_blockchain_message_and_store_as_processed(
self, consumer_address: ChecksumAddress
) -> BlockchainMessage | None:
"""
Method for fetching the oldest unprocessed transaction sent to the consumer address.
After being fetched, it is stored in the DB as processed.
"""
- transactions = self.fetch_unseen_transactions(consumer_address=consumer_address)
- if not transactions:
+ transactions = self.fetch_unseen_transactions(consumer_address=consumer_address, limit=1)
+ if not transactions:
return None
blockchain_message = transactions[0]
# Mark unseen transaction as processed in DB
self.blockchain_table_handler.save_multiple([blockchain_message])
return blockchain_message
Adjust fetch_unseen_transactions
to accept a limit
parameter and modify the query accordingly to fetch only the required transaction.
Committable suggestion skipped: line range outside the PR's diff.
st.markdown( | ||
f""" | ||
**From:** {transaction.sender_address} | ||
**Message:** {transaction.data_field} | ||
**Value:** {wei_to_xdai(transaction.value_wei_parsed)} xDai | ||
""" | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ensure proper sanitization when displaying user-generated content
Displaying transaction.data_field
, which may contain user-generated content, using st.markdown
can pose security risks if the content includes malicious code.
Verify that st.markdown
is safe from injection attacks in this context. If necessary, escape or sanitize transaction.data_field
before rendering:
import html
st.markdown(
f"""
**From:** {transaction.sender_address}
**Message:** {html.escape(transaction.data_field)}
**Value:** {wei_to_xdai(transaction.value_wei_parsed)} xDai
"""
)
This ensures that any HTML or special characters in data_field
are properly escaped, mitigating potential security vulnerabilities.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
prediction_market_agent/db/blockchain_transaction_fetcher.py (1)
Line range hint
44-72
: Protect against SQL injection and optimize query performanceThe SQL query construction using string formatting is vulnerable to SQL injection. Additionally, fetching all columns (*) may impact performance unnecessarily.
- Use parameterized queries to prevent SQL injection:
-query = f'select * from gnosis.transactions where "to" = {Web3.to_checksum_address(consumer_address)} AND block_number >= {min_block_number} and value >= {xdai_to_wei(MicrochainAgentKeys().RECEIVER_MINIMUM_AMOUNT)} order by block_time asc' +query = ''' + SELECT hash, value, block_number, "from", data, block_time + FROM gnosis.transactions + WHERE "to" = :consumer_address + AND block_number >= :min_block_number + AND value >= :min_value + ORDER BY block_time ASC +''' +params = { + "consumer_address": Web3.to_checksum_address(consumer_address), + "min_block_number": min_block_number, + "min_value": xdai_to_wei(MicrochainAgentKeys().RECEIVER_MINIMUM_AMOUNT) +} +df = spice.query(query, params, api_key=keys.dune_api_key.get_secret_value())
- Consider adding a LIMIT clause to prevent memory issues with large result sets:
# Add to query string: LIMIT 1000 # Adjust based on your needs
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
prediction_market_agent/db/blockchain_transaction_fetcher.py
(4 hunks)
🔇 Additional comments (3)
prediction_market_agent/db/blockchain_transaction_fetcher.py (3)
32-42
: 🛠️ Refactor suggestion
Add robust error handling for dictionary access
The direct dictionary key access without validation is still a concern, as previously noted. Additionally, the method should validate the data types of the values retrieved.
def blockchain_message_from_dune_df_row(
self, consumer_address: ChecksumAddress, x: dict[str, Any]
) -> BlockchainMessage:
+ try:
+ # Validate required fields exist and have correct types
+ required_fields = {"hash", "value", "block_number", "from", "data"}
+ if missing := required_fields - x.keys():
+ raise ValueError(f"Missing required fields: {missing}")
+
return BlockchainMessage(
consumer_address=consumer_address,
- transaction_hash=x["hash"],
- value_wei=str(x["value"]),
- block=str(x["block_number"]),
- sender_address=x["from"],
- data_field=self.unzip_message_else_do_nothing(x["data"]),
+ transaction_hash=x.get("hash", ""),
+ value_wei=str(x.get("value", 0)),
+ block=str(x.get("block_number", 0)),
+ sender_address=x.get("from", ""),
+ data_field=self.unzip_message_else_do_nothing(x.get("data", "")),
)
+ except Exception as e:
+ raise ValueError(f"Failed to create BlockchainMessage from row: {e}") from e
77-78
: 🛠️ Refactor suggestion
Optimize transaction count retrieval
Fetching all transactions just to get the count is inefficient, as previously noted. A COUNT query would be more performant.
87-96
: 🛠️ Refactor suggestion
Optimize single transaction fetch and add error handling
Fetching all transactions when only one is needed is inefficient, as previously noted. Additionally, the save operation should include error handling.
def fetch_one_unprocessed_blockchain_message_and_store_as_processed(
self, consumer_address: ChecksumAddress
) -> BlockchainMessage | None:
"""
Method for fetching oldest unprocessed transaction sent to the consumer address.
After being fetched, it is stored in the DB as processed.
"""
- transactions = self.fetch_unseen_transactions(consumer_address=consumer_address)
+ transactions = self.fetch_unseen_transactions(consumer_address=consumer_address, limit=1)
if not transactions:
return None
blockchain_message = transactions[0]
- # mark unseen transaction as processed in DB
- self.blockchain_table_handler.save_multiple([blockchain_message])
+ try:
+ # mark unseen transaction as processed in DB
+ self.blockchain_table_handler.save_multiple([blockchain_message])
+ except Exception as e:
+ # Log the error but don't raise to prevent reprocessing the same transaction
+ logger.error(f"Failed to mark transaction as processed: {e}")
+
return blockchain_message
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
prediction_market_agent/db/blockchain_transaction_fetcher.py (1)
Line range hint
44-70
: Prevent SQL injection by using parameterized queriesThe current implementation uses string interpolation for SQL query parameters, which could be vulnerable to SQL injection attacks. While the risk might be lower since the inputs are processed through Web3.to_checksum_address(), it's still best practice to use parameterized queries.
Consider using Dune API's parameter binding mechanism if available, or at minimum, implement proper input sanitization:
-query = f'select * from gnosis.transactions where "to" = {Web3.to_checksum_address(consumer_address)} AND block_number >= {min_block_number} and value >= {xdai_to_wei(MicrochainAgentKeys().RECEIVER_MINIMUM_AMOUNT)} order by block_time asc' +# Example using parameters (adjust syntax based on spice/Dune API capabilities) +query = ''' + select * from gnosis.transactions + where "to" = :address + AND block_number >= :min_block + and value >= :min_value + order by block_time asc +''' +params = { + "address": Web3.to_checksum_address(consumer_address), + "min_block": min_block_number, + "min_value": xdai_to_wei(MicrochainAgentKeys().RECEIVER_MINIMUM_AMOUNT) +} +df = spice.query(query, params, api_key=keys.dune_api_key.get_secret_value())Additionally, consider adding error handling for the Dune API call and documentation for the query's assumptions about block ordering.
♻️ Duplicate comments (3)
prediction_market_agent/db/blockchain_transaction_fetcher.py (3)
32-42
:⚠️ Potential issueHandle potential KeyError exceptions when accessing dictionary keys
Direct dictionary access without validation could cause crashes when processing incomplete data from Dune API.
Previous review comment still applies - consider using dictionary.get() with default values to handle missing keys safely.
75-76
: 🛠️ Refactor suggestionOptimize transaction counting to avoid fetching full records
Previous review comment about performance optimization still applies - fetching all transaction data just to get the count is inefficient. Consider implementing the suggested COUNT query approach.
85-94
: 🛠️ Refactor suggestionOptimize single message fetching to avoid retrieving all transactions
Previous review comment about optimizing single transaction fetching still applies. Consider implementing the suggested LIMIT 1 approach to avoid unnecessary data retrieval.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
prediction_market_agent/db/blockchain_message_table_handler.py
(1 hunks)prediction_market_agent/db/blockchain_transaction_fetcher.py
(4 hunks)
return self.sql_handler.save_multiple( | ||
# Re-create the items to avoid SQLModel errors. This is a workaround. It's weird, but it works. :shrug: | ||
[BlockchainMessage(**i.model_dump()) for i in items] | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Consider documenting the SQLModel issue and exploring alternative solutions
While the workaround functions, recreating objects for every save operation could impact performance with large datasets. Additionally, the casual comment style (":shrug:") should be replaced with proper documentation.
Consider:
- Document the specific SQLModel error being worked around
- Investigate the root cause - is it related to SQLModel's handling of nested models or serialization?
- Add error handling in case model_dump() fails
- Replace the casual comment with proper technical documentation
def save_multiple(self, items: t.Sequence[BlockchainMessage]) -> None:
return self.sql_handler.save_multiple(
- # Re-create the items to avoid SQLModel errors. This is a workaround. It's weird, but it works. :shrug:
+ # TODO: Temporary workaround for SQLModel serialization issue #<issue_number>
+ # When saving BlockchainMessage instances directly, SQLModel raises:
+ # <specific error message>
+ # This recreates clean instances to avoid the error.
[BlockchainMessage(**i.model_dump()) for i in items]
)
Committable suggestion skipped: line range outside the PR's diff.